Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(sqla_factory): added __set_association_proxy__ attribute #624

Closed
wants to merge 10 commits into from
Closed

feat(sqla_factory): added __set_association_proxy__ attribute #624

wants to merge 10 commits into from

Conversation

nisemenov
Copy link
Contributor

Description

  • an async context manager has been added in SQLAASyncPersistence class to prevent
...
raise RuntimeError('Event loop is closed')
RuntimeError: Event loop is closed

The garbage collector is trying to clean up non-checked-in connection <AdaptedConnection <asyncpg.connection.Connection
object>>, which will be terminated.  Please ensure that SQLAlchemy pooled connections are returned to the pool 
explicitly, either by calling ``close()`` or by using appropriate context managers to manage their lifecycle.

sys:1: SAWarning: The garbage collector is trying to clean up non-checked-in connection <AdaptedConnection 
<asyncpg.connection.Connection object>>, which will be terminated.  Please ensure that SQLAlchemy pooled connections are 
returned to the pool explicitly, either by calling ``close()`` or by using appropriate context managers to manage 
their lifecycle.
  • build_async and batch_async methods have been added to work with Coroutine type field and to send them to the event pool;
  • __set_association_proxy__ attribute has been added in SQLAlchemyFactory class to add AssociationProxy type fields in the result FieldMeta factory dict.

@nisemenov nisemenov requested a review from guacs as a code owner December 27, 2024 11:06
adhtruong
adhtruong reviewed Dec 28, 2024
View reviewed changes
polyfactory/factories/base.py
"""
data: dict[str, Any] = cls.process_kwargs(**kwargs)
for k, v in data.items():
if isinstance(v, Coroutine):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this behaviour generally used for other async methods or more specific to SQLA relationships? If specific to SQLA then should live within that factory

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should check a bit, but could suppose this is more specific for SQLAlchemy proxy. If this behaviour is only for the Alchemy, I'll try to think something and leave it within SQLAlchemy factory.

polyfactory/factories/sqlalchemy_factory.py
@@ -213,6 +218,23 @@ def get_model_fields(cls) -> list[FieldMeta]:
random=cls.__random__,
),
)
if cls.__set_association_proxy__:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Adding as a feature makes sense here. Can appropriate tests and documentation be added for this feature please?

Copy link
Contributor Author

@nisemenov nisemenov Dec 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Ok, I'd try to do that.

@nisemenov
Copy link
Contributor Author

nisemenov commented Jan 14, 2025 •
edited
Loading

Hi, @adhtruong!

I have done some changes in 3 commits:

  • turned __set_foreign_keys__ default value to False, because ForeignKey fields serves only relationship and basically we do not need them without relationship; for example, with True and __set_relationships__ = True, the Factory should do some extra work for creating ForeignKey fields, they will be overwritten after session.add;
  • decided to leave "awaiting coroutine" in base.py, because it is important thing when we work in async style with not only SQLA, for instance with Beanie (added particular test); or if we need more complex async logic for any Factory, when describe field like "@classmethod" with an explicit call DB, etc.;
  • added couple of subsections in SQLAlchemy Usage Guide.

UPD: Lots of checks were not successful because of testing SQLAlchemy 1.4 (my new models are described in v2 style), I will correct them.

@github-actions GitHub Actions
Copy link

Documentation preview will be available shortly at https://litestar-org.github.io/polyfactory-docs-preview/624

@adhtruong
Copy link
Collaborator

Hi, @adhtruong!

I have done some changes in 3 commits:

  • turned __set_foreign_keys__ default value to False, because ForeignKey fields serves only relationship and basically we do not need them without relationship; for example, with True and __set_relationships__ = True, the Factory should do some extra work for creating ForeignKey fields, they will be overwritten after session.add;
  • decided to leave "awaiting coroutine" in base.py, because it is important thing when we work in async style with not only SQLA, for instance with Beanie (added particular test); or if we need more complex async logic for any Factory, when describe field like "@classmethod" with an explicit call DB, etc.;
  • added couple of subsections in SQLAlchemy Usage Guide.

UPD: Lots of checks were not successful because of testing SQLAlchemy 1.4 (my new models are described in v2 style), I will correct them.

Thanks for updating!

  1. Changing the default of __set_foreign_keys__ is breaking behaviour. I think default could be changed but best left to a separate PR and review with other settings. Note a foreign key may be present without a relationship in ORM definition. This default may make less sense now but this was implemented before adding relationship support. Might be better to change these settings together with next major bump
  2. I am not sure on this. Coroutines can not be reused so factories in the test cases will error if try to build_async twice . The test cases should work if initial construction was sync i.e build used throughout. This doesn't cover the async use case for more complex but this is separate feature so can be fully reviewed there and tested separately as a feature rather than with this.

@nisemenov
Copy link
Contributor Author

Thanks for updating!

  1. Changing the default of __set_foreign_keys__ is breaking behaviour. I think default could be changed but best left to a separate PR and review with other settings. Note a foreign key may be present without a relationship in ORM definition. This default may make less sense now but this was implemented before adding relationship support. Might be better to change these settings together with next major bump
  2. I am not sure on this. Coroutines can not be reused so factories in the test cases will error if try to build_async twice . The test cases should work if initial construction was sync i.e build used throughout. This doesn't cover the async use case for more complex but this is separate feature so can be fully reviewed there and tested separately as a feature rather than with this.

Thanks for answering!

Ok, I see it. Then I am going to leave only added __set_association_proxy__ attribute issue in one PR. I need to think more carefully about my other changings.

Could you please help me to do this correctly? May be closing this PR and opening a new one with only __set_association_proxy__ would be more convenient?

@nisemenov nisemenov closed this Jan 15, 2025
This was referenced Jan 15, 2025
Merged
Merged
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adhtruong adhtruong adhtruong left review comments

@guacs guacs Awaiting requested review from guacs guacs is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nisemenov @adhtruong